Make array parsing parameters error message consistency with ZPP failure #3429
Conversation
|
In my opinion, this can go into PHP-7.3. (Minor issue: I'd like more consistency with ZPP failure messages, but wouldn't know how to accomplish this.) |
|
@cmb69 Good idea. Maybe: -Argument #2 should be an array, int given
+array_map() expects parameter 2 to be array, int givenWould be tricky to catch the function name for shared internal functions, but there must be a way. |
|
Hmm, maybe |
|
@cmb69 Also, should we throw a |
carusogabriel
changed the title
|
@carusogabriel Hmm, not sure about the BC break – some may not like that. |
|
@cmb69 I've kept the |
ext/standard/array.c
Outdated
| @@ -6152,7 +6152,7 @@ PHP_FUNCTION(array_map) | |||
|
|
|||
| for (i = 0; i < n_arrays; i++) { | |||
| if (Z_TYPE(arrays[i]) != IS_ARRAY) { | |||
| php_error_docref(NULL, E_WARNING, "Argument #%d should be an array", i + 2); | |||
| php_error_docref(NULL, E_WARNING, "Expected parameter #%d to be an array, %s given", i + 2, zend_get_type_by_const(Z_TYPE(arrays[0]))); | |||
There was a problem hiding this comment.
# should be stripped here as well, or the other wayarround.
I liked the # personally
There was a problem hiding this comment.
I missed this one, thanks.
| @@ -3,7 +3,7 @@ Test array_diff_key() function : usage variation - Passing unexpected values to | |||
| --FILE-- | |||
| <?php | |||
| /* Prototype : array array_diff_key(array arr1, array arr2 [, array ...]) | |||
| * Description: Returns the entries of arr1 that have keys which are not present in any of the others arguments. | |||
| * Description: Returns the entries of arr1 that have keys which are not present in any of the others arguments. | |||
There was a problem hiding this comment.
Please do not submit unrelated whitespace changes! I fully agree that there shouldn't be any trailing whitespace, but automatically removing these may make diffs harder to read, and may cause merge conflicts. If at all, such whitespace changes should go to a separate commit/PR.
There was a problem hiding this comment.
@cmb69 Gonna revert these changes, sorry. We do need to find a way to trim all those whitespaces in tests, btw
ext/standard/array.c
Outdated
| @@ -4639,7 +4639,7 @@ static void php_array_intersect_key(INTERNAL_FUNCTION_PARAMETERS, int data_compa | |||
|
|
|||
| for (i = 0; i < argc; i++) { | |||
| if (Z_TYPE(args[i]) != IS_ARRAY) { | |||
| php_error_docref(NULL, E_WARNING, "Argument #%d is not an array", i + 1); | |||
| php_error_docref(NULL, E_WARNING, "Expected parameter %d to be an array, %s given", i + 1, zend_get_type_by_const(Z_TYPE(args[i]))); | |||
There was a problem hiding this comment.
Any reason to use zend_zval_type_name in some places and zend_get_type_by_const in others?
There was a problem hiding this comment.
I used zend_zval_type_name when possible as was the function I found for such a task. But, as it needs a const zval *arg I got an error type when using here, for e.g.:
expected ‘const zval * {aka const struct _zval_struct *}’ but argument is of type ‘zval {aka struct _zval_struct}’
So I search a way to retrieve the type, and come up with zend_get_type_by_const + Z_TYPE. Is there a better way? :)
There was a problem hiding this comment.
You can get a zval* from a zval by writing &args[i] instead of args[i].
There was a problem hiding this comment.
Me and my C deficit 🤦♂️
|
@cmb69 May we merge this one? |
|
Maybe wait for a few days, so others are able to object. If nobody objects, it's fine to merge. |
|
If there's no objection on this one, I'll merge on August 27th, so we can include it in PHP 7.3 beta 3. |
|
@carusogabriel Feel free to merge right away, I think enough time has passed for anyone to object. |
|
Merged as efbf846, thanks! |
As suggested in #76674, we could improve our parsing parameters error messages for
array_functions, by exposing what was passed, instead of an array.I'm opening the PR for review before merging, as well asking if this should go for 7.3 or wait for 7.4 :)